C++: Model taint through many more methods in std::string#4090
C++: Model taint through many more methods in std::string#4090jbj merged 6 commits intogithub:mainfrom
Conversation
| // flow from string and parameter to string (qualifier) and return value | ||
| ( | ||
| input.isQualifierObject() or | ||
| input.isParameterDeref(getAStringParameter()) |
There was a problem hiding this comment.
Will this correctly classify taint from iterators? i.e., will we get taint to s2 in the following case:
std::string s = source();
std::string s2;
s2.append(s.begin(), s.end());or do we need to extend this predicate once we have a model for iterators?
There was a problem hiding this comment.
The intent for this particular PR is not to model taint through the iterator versions of the functions. I wanted to see what's decided / done for https://github.com/github/codeql-c-analysis-team/issues/122 before addressing them.
|
|
||
| override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) { | ||
| // flow from parameter to string itself (qualifier) and return value | ||
| input.isParameterDeref(getAStringParameter()) and |
There was a problem hiding this comment.
Likewise (wrt. the iterator overload).
jbj
left a comment
jbj
left a comment
There was a problem hiding this comment.
LGTM apart from one question
| * The standard function `std::string.swap`. | ||
| */ | ||
| class StdStringSwap extends TaintFunction { | ||
| StdStringSwap() { this.hasQualifiedName("std", "basic_string", "swap") } |
There was a problem hiding this comment.
Shouldn't we have a general model of members named swap? Or did we only do that for std::swap?
There was a problem hiding this comment.
Yep, we only did std::swap and in this case the parameters are different. std::swap(a, b) vs a.swap(b).
3 participants
Model taint through many more methods in
std::string. The false positives are situations where we want to block taint, but we don't presently have the ability for models to do that.